Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Added getAuthIdentifierForBroadcasting method #37408

Merged
merged 3 commits into from
May 19, 2021
Merged

[8.x] Added getAuthIdentifierForBroadcasting method #37408

merged 3 commits into from
May 19, 2021

Conversation

yousuf-hossain-shanto
Copy link
Contributor

@yousuf-hossain-shanto yousuf-hossain-shanto commented May 19, 2021

I am working on a project where doctor can chat with patients. Doctor and Patient are two different auth guard and different model also. I am using pusher and Laravel echo for real-time chat.

Everything is fine when doctor and patient id is different. But facing issue when doctor and patient id is same. But, doctor and patient id can be same because of they are from different model and different database table.

Currently Laravel is sending getAuthIdentifier method value to broadcaster (pusher, redis) and those broadcaster is identifying users by this value. It can be resolved by modifying getAuthIdentifier method into relevant authenticatable model. But, getAuthIdentifier method is a common used method by various packages and libraries. They expect id from using it. So, it is risky to modify it.

I have added a new method named getAuthIdentifierForBroadcasting to resolve it.

Thanks in advance

@GrahamCampbell GrahamCampbell changed the title Added getAuthIdentifierForBroadcasting method [8.x] Added getAuthIdentifierForBroadcasting method May 19, 2021
@morloderex
Copy link
Contributor

morloderex commented May 19, 2021

Another way of solving this is having to different guards e.g doctors and patients.

Then you could define your channels like this which signal's the broadcaster to use both guards:

Broadcast::channel('some-doctor-patient-channel-name', function ($user, $id) {
   // determine if the channel is accessible for a given user model here
}, ['guard' => 'doctor', 'patient']);

You do unfortunetly need to define this guard configuration for every channel you have currently there is no way of doing this on a global level e.g for all channels as far as i can see from the source code.

Based on this information i do not think your suggestion is necessary needed as one could just define which guard(s) a specific channel should use instead.

See relevant code here and here

@yousuf-hossain-shanto
Copy link
Contributor Author

yousuf-hossain-shanto commented May 19, 2021

Another way of solving this is having to different guards e.g doctors and patients.

Then you could define your channels like this which signal's the broadcaster to use both guards:

Broadcast::channel('some-doctor-patient-channel-name', function ($user, $id) {
   // determine if the channel is accessible for a given user model here
}, ['guard' => 'doctor', 'patient']);

You do unfortunetly need to define this guard configuration for every channel you have currently there is no way of doing this on a global level e.g for all channels as far as i can see from the source code.

Based on this information i do not think your suggestion is necessary needed as one could just define which guard(s) a specific channel should use instead.

I already authenticate my channel using this way but that issue is happening after authentication. I debug on pusher and see that,

  1. Doctor (id 1) joined to a channel chat
  2. Patients (id 1) joined to the same channel

and after that, pusher/redis identify the last user joined at that channel because of their same id (Laravel sent as user_id). Broadcaster not firing member_added (pusher) event because they understand the user already on this channel (because of their same id)

@morloderex
Copy link
Contributor

morloderex commented May 19, 2021

Ah, i get your point now, my bad for not understanding completly what you meant.

Adding this method might be useful then, however would a sensible default value be to add the retrieved model's class name as a prefix to the model's identifier something like {guard_model_class_name}:{guard_model_identifier}. This way it should work by default when using mutiple guards?

@yousuf-hossain-shanto
Copy link
Contributor Author

yousuf-hossain-shanto commented May 19, 2021

@morloderex Thanks for your suggestion. I just added a different method with previous return value (model id). If one need to prefix model identifier with something, then he/she can change it as he/she want. Otherwise default value (model id) will be used as Laravel do it currently

@taylorotwell
Copy link
Member

For this to not be breaking we may need to ensure that the new method exists on the model before calling it. Some people may not be using the Authenticatable trait and may be implementing the getAuthIdentifier method themselves.

@taylorotwell
Copy link
Member

Please revert all those spacing changes.

@yousuf-hossain-shanto
Copy link
Contributor Author

Now it will use default getAuthIdentifier method as a fallback

@taylorotwell taylorotwell merged commit f93d8d8 into laravel:8.x May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants